HDDS-6194. EC: Freon ockg support EC write #2996

captainzmc · 2022-01-18T08:30:03Z

What changes were proposed in this pull request?

see: https://issues.apache.org/jira/browse/HDDS-6194

How was this patch tested?

NA.

sodonnel

LGTM

adoroszlai · 2022-01-18T16:57:59Z

hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/OzoneClientKeyGenerator.java

It would be nice to keep the old option as @Deprecated for backward compatibility. It should imply --type RATIS, and be mutually exclusive with --replication.

umamaheswararao · 2022-01-18T17:55:51Z

hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/OzoneClientKeyGenerator.java

STANDALONE --> STAND_ALONE ?

umamaheswararao · 2022-01-18T18:00:15Z

hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/OzoneClientKeyGenerator.java

parse is returning default values even if we pass replication and type as null.
When we set the EC on bucket, ideally it's not mandatory to pass the type and replication with key as we moved the default configs to server side along with bucket.

Please refer the JIRA: HDDS-6184

captainzmc · 2022-01-19T02:30:53Z

Thanks @sodonnel @adoroszlai @umamaheswararao for the review, while #2990 merged I will update this PR.

umamaheswararao · 2022-01-28T19:14:03Z

HI @captainzmc, JFYI The pr #2990 is committed now. You may want to resume your work. Thanks

kaijchen · 2022-02-10T09:35:14Z

Hi @adoroszlai @sodonnel @umamaheswararao, since @captainzmc is on vacation, I have updated the code according to your comments above. Please take another look, thanks.

adoroszlai

Thanks @kaijchen for updating the patch.

adoroszlai · 2022-02-10T10:51:31Z

hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/OzoneClientKeyGenerator.java

-              ReplicationType.RATIS, factor, metadata)) {
+          replicationConfig, metadata)) {
        contentGenerator.write(stream);
        stream.flush();


Got this for EC key:

$ ozone freon ockg -n1 -t1 --replication 'rs-3-2-1024k' --type EC ... NotImplementedException: The flush API is not implemented yet. at org.apache.hadoop.ozone.client.io.ECKeyOutputStream.flush(ECKeyOutputStream.java:468) at org.apache.hadoop.ozone.client.io.OzoneOutputStream.flush(OzoneOutputStream.java:55) at org.apache.hadoop.ozone.freon.OzoneClientKeyGenerator.lambda$createKey$0(OzoneClientKeyGenerator.java:145) at com.codahale.metrics.Timer.time(Timer.java:101) at org.apache.hadoop.ozone.freon.OzoneClientKeyGenerator.createKey(OzoneClientKeyGenerator.java:141)

Yes, this is expected. It should be fixed in another PR.

ozone/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/ECKeyOutputStream.java

Lines 466 to 469 in 22e3482

@Override

public void flush() {

throw new NotImplementedException("The flush API is not implemented yet.");

}

Should we catch this exception here in ockg?

Probably for now if Type is EC, ignore to invoke flush? [ We thought not to implement flush unless there is a critical req for it. ]

Probably for now if Type is EC, ignore to invoke flush? [ We thought not to implement flush unless there is a critical req for it. ]

Maybe just catch this exception? Since it's already in a try clause.
And if flush is implemented in the future, it will work without any change.

adoroszlai · 2022-02-10T10:55:38Z

hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/OzoneClientKeyGenerator.java

+    if (spec.commandLine().getParseResult().hasMatchedOption("--factor")) {
+      replicationConfig = ReplicationConfig
+          .fromTypeAndFactor(ReplicationType.RATIS, factor);
+    } else {
+      replicationConfig = OzoneClientUtils
+          .validateAndGetClientReplicationConfig(replicationType, replication,
+              ozoneConfiguration);
+    }


Something is wrong around option handling. Setting only replication, but neither factor nor type, a regular RATIS/3 key was created:

$ ozone freon ockg -n1 -t1 --replication 'rs-3-2-1024k' -p replication_rs-3-2-1024k ... Successful executions: 1 $ ozone sh key info /vol1/bucket1/replication_rs-3-2-1024k/0 ... "replicationConfig" : { "replicationFactor" : "THREE", "requiredNodes" : 3, "replicationType" : "RATIS" }, ...

It is meaningless to set replication or type only.
And it's also possible to set --type EC with --replication THREE together.

type/replication THREE rs-3-2-1024k

RATIS OK Bad

EC Bad OK

Should we bind this two options together like RATIS/THREE or EC/rs-3-2-1024k instead?

Something is wrong around option handling. Setting only replication, but neither factor nor type, a regular RATIS/3 key was created:

This is due to OzoneClientUtils#validateAndGetClientReplicationConfig, the default client config was used.

ozone/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/OzoneClientUtils.java

Lines 172 to 175 in c4aec7b

if (userPassedType == null && clientConfiguredDefaultType != null) {

clientReplicationType =

ReplicationType.valueOf(clientConfiguredDefaultType);

}

…sh()" This reverts commit d9ed09a.

umamaheswararao · 2022-02-11T07:49:40Z

Yes, that should be fine.

umamaheswararao

LGTM

umamaheswararao · 2022-02-11T15:24:33Z

@adoroszlai do you any more comments? Otherwise I will just go ahead to merge.

adoroszlai

Thanks @kaijchen for updating the patch. Works fine in valid combinations. I think we can add validations later.

kaijchen · 2022-02-12T02:39:19Z

Thanks @umamaheswararao and @adoroszlai for reviewing.

captainzmc requested review from sodonnel and umamaheswararao January 18, 2022 08:36

sodonnel approved these changes Jan 18, 2022

View reviewed changes

adoroszlai self-requested a review January 18, 2022 16:52

adoroszlai reviewed Jan 18, 2022

View reviewed changes

umamaheswararao reviewed Jan 18, 2022

View reviewed changes

captainzmc and others added 2 commits February 10, 2022 15:26

freon ockg support EC

bb24b76

ockg: enable bucket-level replication config

58a9c54

kaijchen force-pushed the HDDS-3816-ec-0110 branch from 04ca2ce to 58a9c54 Compare February 10, 2022 09:01

kaijchen added 2 commits February 10, 2022 17:05

fix style

a918ce3

ockg: restore --factor option for backward compatibility

01b5181

fix checkstyle

8e9793d

adoroszlai reviewed Feb 10, 2022

View reviewed changes

kaijchen added 2 commits February 11, 2022 14:57

ockg: catch NotImplementedException for ECKeyOutputStream#flush()

d9ed09a

Revert "ockg: catch NotImplementedException for ECKeyOutputStream#flu…

287a177

…sh()" This reverts commit d9ed09a.

ockg: do not call flush() for EC Keys

ac6aa13

kaijchen force-pushed the HDDS-3816-ec-0110 branch from aab08ef to ac6aa13 Compare February 11, 2022 07:56

trigger CI

abb01da

umamaheswararao approved these changes Feb 11, 2022

View reviewed changes

adoroszlai approved these changes Feb 11, 2022

View reviewed changes

umamaheswararao merged commit 2d63cc4 into apache:HDDS-3816-ec Feb 11, 2022

	@Override
	public void flush() {
	throw new NotImplementedException("The flush API is not implemented yet.");
	}

	if (userPassedType == null && clientConfiguredDefaultType != null) {
	clientReplicationType =
	ReplicationType.valueOf(clientConfiguredDefaultType);
	}

HDDS-6194. EC: Freon ockg support EC write #2996

HDDS-6194. EC: Freon ockg support EC write #2996

Uh oh!

Conversation

captainzmc commented Jan 18, 2022

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

sodonnel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

captainzmc commented Jan 19, 2022

Uh oh!

umamaheswararao commented Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaijchen commented Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaijchen Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

umamaheswararao commented Feb 11, 2022

Uh oh!

umamaheswararao left a comment

Choose a reason for hiding this comment

Uh oh!

umamaheswararao commented Feb 11, 2022

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

kaijchen commented Feb 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

umamaheswararao commented Jan 28, 2022 •

edited

Loading

kaijchen commented Feb 10, 2022 •

edited

Loading

kaijchen Feb 10, 2022 •

edited

Loading